feat: add language and instructions support for mind maps#277
Open
voidborne-d wants to merge 1 commit intoteng-lin:mainfrom
Open
feat: add language and instructions support for mind maps#277voidborne-d wants to merge 1 commit intoteng-lin:mainfrom
voidborne-d wants to merge 1 commit intoteng-lin:mainfrom
Conversation
📝 WalkthroughWalkthroughThe changes extend mind map generation functionality with Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to specify a language and custom instructions when generating mind maps. The changes include updating the generate_mind_map method in the artifacts API to accept these new parameters, adding a description argument and --language option to the CLI, and including comprehensive unit tests for both the API and CLI layers. Feedback was provided to refactor the CLI logic to avoid redundant calls and improve code clarity by resolving parameters once before use.
Comment on lines
1021
to
1035
| if json_output: | ||
| result = await client.artifacts.generate_mind_map( | ||
| nb_id_resolved, source_ids=sources | ||
| nb_id_resolved, | ||
| source_ids=sources, | ||
| language=resolve_language(language), | ||
| instructions=description or None, | ||
| ) | ||
| else: | ||
| with console.status("Generating mind map..."): | ||
| result = await client.artifacts.generate_mind_map( | ||
| nb_id_resolved, source_ids=sources | ||
| nb_id_resolved, | ||
| source_ids=sources, | ||
| language=resolve_language(language), | ||
| instructions=description or None, | ||
| ) |
Contributor
There was a problem hiding this comment.
The logic for resolving the language and instructions is duplicated across the if/else branches. Additionally, calling resolve_language multiple times within the same function can be inefficient if it involves file I/O. It's better to resolve these values once and reuse them to improve efficiency and clarity.
Suggested change
| if json_output: | |
| result = await client.artifacts.generate_mind_map( | |
| nb_id_resolved, source_ids=sources | |
| nb_id_resolved, | |
| source_ids=sources, | |
| language=resolve_language(language), | |
| instructions=description or None, | |
| ) | |
| else: | |
| with console.status("Generating mind map..."): | |
| result = await client.artifacts.generate_mind_map( | |
| nb_id_resolved, source_ids=sources | |
| nb_id_resolved, | |
| source_ids=sources, | |
| language=resolve_language(language), | |
| instructions=description or None, | |
| ) | |
| lang = resolve_language(language) | |
| instr = description or None | |
| if json_output: | |
| result = await client.artifacts.generate_mind_map( | |
| nb_id_resolved, | |
| source_ids=sources, | |
| language=lang, | |
| instructions=instr, | |
| ) | |
| else: | |
| with console.status("Generating mind map..."): | |
| result = await client.artifacts.generate_mind_map( | |
| nb_id_resolved, | |
| source_ids=sources, | |
| language=lang, | |
| instructions=instr, | |
| ) |
References
- To improve efficiency and clarity, avoid reading the same file multiple times within a single function. Instead, read the file once, store its contents in a variable, and reuse the variable.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notebooklm/cli/generate.py`:
- Around line 997-998: Add a new Click option --instructions (e.g.,
`@click.option`("--instructions", default=None, help="Mind-map instructions;
overrides positional description")) alongside the existing positional argument
"description" and "--language"; then update the command handler to prefer the
--instructions value when present and fall back to the positional description
for backward compatibility (replace usages that currently read the positional
"description" at the call sites referenced around lines 1002, 1023-1026, and
1031-1034 so they use instructions = instructions if instructions is not None
else description). Ensure the option help text clearly states it overrides the
positional description.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cfdc7b29-52c4-4a38-a582-35a4febf2af5
📒 Files selected for processing (4)
src/notebooklm/_artifacts.pysrc/notebooklm/cli/generate.pytests/unit/cli/test_generate.pytests/unit/test_source_selection.py
Comment on lines
+997
to
+998
| @click.argument("description", default="", required=False) | ||
| @click.option("--language", default=None, help="Output language (default: from config or 'en')") |
There was a problem hiding this comment.
Add an explicit --instructions option for mind-map generation.
The implementation currently supports instructions only via positional description, but the stated objective requires a dedicated --instructions flag.
Proposed fix (keep positional text for backward compatibility)
`@generate.command`("mind-map")
`@click.option`(
@@
)
`@click.argument`("description", default="", required=False)
+@click.option(
+ "--instructions",
+ default=None,
+ help="Custom instructions for mind map generation",
+)
`@click.option`("--language", default=None, help="Output language (default: from config or 'en')")
`@click.option`("--source", "-s", "source_ids", multiple=True, help="Limit to specific source IDs")
`@json_option`
`@with_client`
-def generate_mind_map(ctx, notebook_id, description, language, source_ids, json_output, client_auth):
+def generate_mind_map(
+ ctx, notebook_id, description, instructions, language, source_ids, json_output, client_auth
+):
@@
async def _run():
async with NotebookLMClient(client_auth) as client:
nb_id_resolved = await resolve_notebook_id(client, nb_id)
sources = await resolve_source_ids(client, nb_id_resolved, source_ids)
+ resolved_instructions = instructions if instructions is not None else (description or None)
@@
result = await client.artifacts.generate_mind_map(
nb_id_resolved,
source_ids=sources,
language=resolve_language(language),
- instructions=description or None,
+ instructions=resolved_instructions,
)
else:
with console.status("Generating mind map..."):
result = await client.artifacts.generate_mind_map(
nb_id_resolved,
source_ids=sources,
language=resolve_language(language),
- instructions=description or None,
+ instructions=resolved_instructions,
)Also applies to: 1002-1002, 1023-1026, 1031-1034
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notebooklm/cli/generate.py` around lines 997 - 998, Add a new Click
option --instructions (e.g., `@click.option`("--instructions", default=None,
help="Mind-map instructions; overrides positional description")) alongside the
existing positional argument "description" and "--language"; then update the
command handler to prefer the --instructions value when present and fall back to
the positional description for backward compatibility (replace usages that
currently read the positional "description" at the call sites referenced around
lines 1002, 1023-1026, and 1031-1034 so they use instructions = instructions if
instructions is not None else description). Ensure the option help text clearly
states it overrides the positional description.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
languageandinstructionsparameters toArtifactsAPI.generate_mind_map()generate mind-mapCLI through--languageplus optional freeform instructionsTesting
PYTHONPATH=src python3 -m pytest tests/unit/test_source_selection.py tests/unit/cli/test_generate.py -qFixes #250
Summary by CodeRabbit
--languageoption and optional description parameter for enhanced control over generated mind maps.